[libc++] Fix diagnostic for <stdatomic.h> before C++23 #83351
Conversation
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Louis Dionne (ldionne) ChangesWe normally try to issue a reasonable diagnostic when mixing <stdatomic.h> and <atomic> before C++23. However, after granularizing the <atomic> header, the check and the #error message was moved to after the point where mixing both causes problems. When mixing both headers, we would hence get the diagnostic burried under a pile of previous diagnostics in e.g. __atomic/kill_dependency.h. This patch moves the check earlier to restore the intended behavior. It also switches from Full diff: https://github.com/llvm/llvm-project/pull/83351.diff 1 Files Affected:
diff --git a/libcxx/include/atomic b/libcxx/include/atomic
index 2e8f5b521a55eb..ab6fc566aabc02 100644
--- a/libcxx/include/atomic
+++ b/libcxx/include/atomic
@@ -587,6 +587,10 @@ template <class T>
*/
+#if _LIBCPP_STD_VER < 23 && defined(_LIBCPP_STDATOMIC_H)
+# error <atomic> is incompatible with <stdatomic.h> before C++23. Please compile with -std=c++23.
+#endif
+
#include <__assert> // all public C++ headers provide the assertion handler
#include <__atomic/aliases.h>
#include <__atomic/atomic.h>
@@ -613,10 +617,6 @@ template <class T>
# error <atomic> is not implemented
#endif
-#ifdef kill_dependency
-# error <atomic> is incompatible with <stdatomic.h> before C++23. Please compile with -std=c++23.
-#endif
-
#if !defined(_LIBCPP_REMOVE_TRANSITIVE_INCLUDES) && _LIBCPP_STD_VER <= 20
# include <cmath>
# include <compare>
|
We normally try to issue a reasonable diagnostic when mixing <stdatomic.h> and <atomic> before C++23. However, after granularizing the <atomic> header, the check and the #error message was moved to *after* the point where mixing both causes problems. When mixing both headers, we would hence get the diagnostic burried under a pile of previous diagnostics in e.g. __atomic/kill_dependency.h. This patch moves the check earlier to restore the intended behavior. It also switches from `#ifdef kill_dependency` to an explicit check of the inclusion of the header and the Standard version, which seems to be more reliable than checking whether a macro is defined.
ldionne
force-pushed
the
review/stdatomic-diagnostic
branch
from
7e73799 to
b899632
Compare
|
I think this PR won't work with Android. Since 2014 (https://r.android.com/104366), Android has allowed including Maybe disable it for Bionic? -#if _LIBCPP_STD_VER < 23 && defined(_LIBCPP_STDATOMIC_H)
+#if _LIBCPP_STD_VER < 23 && defined(_LIBCPP_STDATOMIC_H) && !defined(__BIONIC__)I'm guessing libc++ doesn't have an Android-specific test verifying that stdatomic.h and atomic can both be used before C++23. I think I could add one. |
|
I'm not sure how that ever worked since we did have an |
|
There are three
The previous libc++ Both the Bionic and the libc++ |
|
I wonder if libc++'s |
|
OTOH, Clang's stdatomic.h delegates to the system stdatomic.h, and maybe we'd want to keep that delegation for Bionic. I don't know why Android LLVM's build script is overwriting Clang's stdatomic.h, though. Maybe we needed it for host builds. I do still wonder if it's possible to enable libc++'s stdatomic-to-atomic delegation for all language modes, though. That could simplify the situation on Android a lot if it worked. |
We normally try to issue a reasonable diagnostic when mixing <stdatomic.h> and before C++23. However, after granularizing the header, the check and the #error message was moved to after the point where mixing both causes problems. When mixing both headers, we would hence get the diagnostic burried under a pile of previous diagnostics in e.g. __atomic/kill_dependency.h.
This patch moves the check earlier to restore the intended behavior. It also switches from
#ifdef kill_dependencyto an explicit check of the inclusion of the header and the Standard version, which seems to be more reliable than checking whether a macro is defined.